Skip to content

Reduce use of madvise DODUMP/DONTDUMP#95643

Merged
kunalspathak merged 8 commits intodotnet:mainfrom
a74nh:reduce_dodump_github
Dec 21, 2023
Merged

Reduce use of madvise DODUMP/DONTDUMP#95643
kunalspathak merged 8 commits intodotnet:mainfrom
a74nh:reduce_dodump_github

Conversation

@a74nh
Copy link
Copy Markdown
Contributor

@a74nh a74nh commented Dec 5, 2023

  • When reserving memory, do not mark as DONTDUMP if the memory will be committed immediately afterwards.
  • When committing memory that has only just been allocated, do not mark as DODUMP (as this is the default).
  • When resetting memory, only call madvise once.

* When reserving memory, do not mark as DONTDUMP if the memory will
  be committed immediately afterwards.
* When committing memory that has only just been allocated, do not
  mark as DODUMP (as this is the default).
* When resetting memory, only call madvise once.
@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-PAL-coreclr labels Dec 5, 2023
@a74nh
Copy link
Copy Markdown
Contributor Author

a74nh commented Dec 5, 2023

As discussed in #87173

@a74nh a74nh marked this pull request as ready for review December 5, 2023 17:09
@a74nh
Copy link
Copy Markdown
Contributor Author

a74nh commented Dec 5, 2023

@kunalspathak

Comment thread src/coreclr/gc/unix/gcenv.unix.cpp Outdated
@kunalspathak
Copy link
Copy Markdown
Contributor

cc: @Maoni0

Comment thread src/coreclr/pal/src/map/virtual.cpp Outdated
@a74nh a74nh force-pushed the reduce_dodump_github branch from fafb0f7 to ab47f4e Compare December 6, 2023 10:00
Comment thread src/coreclr/gc/unix/gcenv.unix.cpp Outdated
Comment thread src/coreclr/gc/unix/gcenv.unix.cpp Outdated
Comment thread src/coreclr/gc/unix/gcenv.unix.cpp Outdated
Comment thread src/coreclr/gc/unix/gcenv.unix.cpp Outdated
// Do not include reset memory in coredump.
madvise(address, size, MADV_DONTDUMP);
// In case the MADV_FREE is not supported, use MADV_DONTNEED
st = posix_madvise(address, size, MADV_DONTNEED);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are missing the MADV_DONTDUMP for this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly MADV_DONTDUMP isn't supported for posix_madvise(), so we need to call madvise() again.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've just found an interesting comment in the posix_madvise doc:

In glibc, this function is implemented using madvise(2).
However, since glibc 2.6, POSIX_MADV_DONTNEED is treated as a no-
op, because the corresponding madvise(2) value, MADV_DONTNEED,
has destructive semantics.

Interestingly, we are passing MADV_DONTNEED instead of POSIX_MADV_DONTNEED to the posix_madvise. I don't know what is the actual result of calling posix_madvise in such case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, we are passing MADV_DONTNEED instead of POSIX_MADV_DONTNEED to the posix_madvise. I don't know what is the actual result of calling posix_madvise in such case.

These are both 4. I've updated to POSIX_MADV_DONTNEED to make it explicit.

i've just found an interesting comment in the posix_madvise doc:

In glibc, this function is implemented using madvise(2).
However, since glibc 2.6, POSIX_MADV_DONTNEED is treated as a no-
op, because the corresponding madvise(2) value, MADV_DONTNEED,
has destructive semantics.

Glibc 2.6 was introduced in 2007. Ubuntu 22.04 has Glibc 2.35.

Given that and all the ifdef protections, it feels very unlikely that calling posix_madvise() after a failing madvise() would ever do anything.

I've updated so that posix_madvise() is only called if MADV_DONTDUMP does not exist.

Comment thread src/coreclr/pal/src/map/virtual.cpp Outdated
Comment thread src/coreclr/pal/src/map/virtual.cpp Outdated
@a74nh
Copy link
Copy Markdown
Contributor Author

a74nh commented Dec 20, 2023

Quick ping on this. Everything should be fixed now.

Copy link
Copy Markdown
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one last comment, otherwise it looks good.

Comment thread src/coreclr/pal/src/map/virtual.cpp Outdated
Copy link
Copy Markdown
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@kunalspathak kunalspathak merged commit 817b4ee into dotnet:main Dec 21, 2023
@a74nh a74nh deleted the reduce_dodump_github branch January 9, 2024 16:39
@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants